[dependency] sdk/log/xlog: Add FilterProcessor and EnabledParameters #6271
Conversation
pellared
changed the title
pellared
added
area:logs
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6271 +/- ##
=====================================
Coverage 81.8% 81.8%
=====================================
Files 283 283
Lines 24892 24900 +8
=====================================
+ Hits 20379 20389 +10
+ Misses 4109 4107 -2
Partials 404 404 |
pellared
requested review from
MrAlias,
XSAM,
dashpole and
dmathieu
as code owners
|
This seems like a good improvement over the previous approach. I don't see any drawbacks, and would be comfortable using this approach elsewhere |
|
I like this approach. This is also how the collector does experimental changes in stable packages, for example with xreceiver. |
I see that |
|
That definitely makes sense. |
Done 4e54827 |
pellared
changed the title
| @@ -10,6 +10,7 @@ require ( | |||
| go.opentelemetry.io/otel v1.34.0 | |||
| go.opentelemetry.io/otel/log v0.10.0 | |||
| go.opentelemetry.io/otel/sdk v1.34.0 | |||
| go.opentelemetry.io/otel/sdk/log/xlog v0.0.0-00010101000000-000000000000 | |||
There was a problem hiding this comment.
Doesn't this mean when this is stabilized, this module will depend on an unstable module we control?
There was a problem hiding this comment.
Yes, but I think the impact would be acceptable.
This would require the users to use versions that are compatible. This would affect mostly users that opt-in to use xlog so that it is a direct dependency. FWIW most people do not bump indirect dependencies.
Personaly, I think it is an acceptable trade-off.
Similarly we depend on unstable github.com/google/go-cmp and golang.org/x/sys and I do not thing it prevents as from stabilization of the modules that use it.
I think it is good to not depend on unstable modules but it is not always feasible.
There was a problem hiding this comment.
Similarly we depend on unstable
github.com/google/go-cmpandgolang.org/x/sysand I do not thing it prevents as from stabilization of the modules that use it.
These are not maintained by us though.
There was a problem hiding this comment.
This would require the users to use versions that are compatible. This would affect mostly users that opt-in to use xlog so that it is a direct dependency. FWIW most people do not bump indirect dependencies.
I think this is the crux of the problem. We need to provide stability guarantees for the SDK we release. I don't think saying "people do not bump indirect dependencies" is a valid way to get around this.
There was a problem hiding this comment.
This seems like it will share the same issue we had with cross-module internal dependencies, right?
There was a problem hiding this comment.
If not then I think I can make a longer list of arguments for Enabled (context, severity, scope, resource) instead of EnabledPararamters (which could be introduced when the feature is going to be stabilized).
Are you suggesting we are going to use the same EnabledParameters for Logger.Enabled and FilterProcessor.Enabled? This will lead us to an awkward position if we need to change FilterProcessor.Enabled after stabilizing the Logger.
I think we can copy experimental interface definitions to the stable module if we want to make it stable. After that, we drop the use of experimental interfaces in SDK. This will break all existing processors that implement the experimental interfaces, but it seems fine, as they are experimental features.
There was a problem hiding this comment.
Are you suggesting we are going to use the same
EnabledParametersfor Logger.Enabled andFilterProcessor.Enabled?
No, we need scope.Instrumentation and resource.Resource. I meant something like
type FiltererProcessor interface {
Enabled(context.Context, log.Severity, scope.Instrumentation, resource.Resource) bool
}There was a problem hiding this comment.
We can use tooling to copy experimental interface definitions to the stable module if we want to ensure syncing, but not having the to-be-stable module depend on the unstable module prevents a set of user bugs and is preferential.
As suspected. It does not work with type assertions. However, I think we can try use reflect instead to make it working.
The benefit of this approach is that it should never break the compilation for the users. It mitigates the problem if they happen to run in a conflict and have incompatible version of xlog (e.g. transient dependencies).
The main drawback for users is no compile-time guarantee for people that opt-in in to an experimental feature.
The other drawback is the complexity of the implementation (on our side) because of using reflection.
The minor drawbacks are a little more overhead
It seems reasonable to favor stability guarantees. It also looks like a safer, less disruptive approach for our users. For sure it is worth creating a PR that tries doing it so that we can compare both approaches.
The next alternative is to have an experimental interface like
type FiltererProcessor interface {
Enabled(context.Context, log.Severity, scope.Instrumentation, resource.Resource) bool
}This way we would be able to use type assertions, but I think it is a bad idea given we would like to introduce EnabledParameters. It would make migration from experimental to stable very awkward and inconvenient. I do not like this and I do not want to go forward. I think this would look very hacky and unprofessional. At last we would not be able to use such approach for more complex parameters.
There was a problem hiding this comment.
#6286 is ready for review.
There was a problem hiding this comment.
I think the performance overhead in the other PR is less acceptable than the potential build failures that users can run into.
|
From the SIG meeting: could we add a check (maybe a lint) to ensure no SDK uses an If we ensure no stable SDK uses the x modules, the only folks who have to upgrade in lockstep are folks who made the conscious choice to use an unstable module. |
| @@ -10,6 +10,7 @@ require ( | |||
| go.opentelemetry.io/otel v1.34.0 | |||
| go.opentelemetry.io/otel/log v0.10.0 | |||
| go.opentelemetry.io/otel/sdk v1.34.0 | |||
| go.opentelemetry.io/otel/sdk/log/xlog v0.0.0-00010101000000-000000000000 | |||
There was a problem hiding this comment.
If not then I think I can make a longer list of arguments for Enabled (context, severity, scope, resource) instead of EnabledPararamters (which could be introduced when the feature is going to be stabilized).
Are you suggesting we are going to use the same EnabledParameters for Logger.Enabled and FilterProcessor.Enabled? This will lead us to an awkward position if we need to change FilterProcessor.Enabled after stabilizing the Logger.
I think we can copy experimental interface definitions to the stable module if we want to make it stable. After that, we drop the use of experimental interfaces in SDK. This will break all existing processors that implement the experimental interfaces, but it seems fine, as they are experimental features.
This PR already makes SDK to use an |
Co-authored-by: Sam Xie <[email protected]>
Co-authored-by: Sam Xie <[email protected]>
pellared
changed the title
pellared
changed the title
|
I reopened this PR because of the huge performance overhead (speed and 16 allocs) of #6286. |
|
SIG meeting notes: |
Per #6271 (comment) > We agreed that we can move `FilterProcessor` directly to `sdk/log` as Logs SDK does not look to be stabilized soon. - Add the possibility to filter based on the resource and scope which is available for the SDK. The scope information is the most important as it gives the possibility to e.g. filter out logs emitted for a given logger. Thus e.g. open-telemetry/opentelemetry-specification#4364 is not necessary. See open-telemetry/opentelemetry-specification#4290 (comment) for more context. - It is going be an example for open-telemetry/opentelemetry-specification#4363 There is a little overhead (IMO totally acceptable) because of data transformation. Most importantly, there is no new heap allocation. ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/sdk/log cpu: 13th Gen Intel(R) Core(TM) i7-13800H │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ LoggerEnabled-20 4.589n ± 1% 319.750n ± 16% +6867.75% (p=0.000 n=10) │ old.txt │ new.txt │ │ B/op │ B/op vs base │ LoggerEnabled-20 0.000Ki ± 0% 1.093Ki ± 13% ? (p=0.000 n=10) │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ LoggerEnabled-20 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal ``` `Logger.Enabled` is still more efficient than `Logger.Emit` (benchmarks from #6315). ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/sdk/log cpu: 13th Gen Intel(R) Core(TM) i7-13800H BenchmarkLoggerEmit/5_attributes-20 559934 2391 ns/op 39088 B/op 1 allocs/op BenchmarkLoggerEmit/10_attributes-20 1000000 5910 ns/op 49483 B/op 5 allocs/op BenchmarkLoggerEnabled-20 1605697 968.7 ns/op 1272 B/op 0 allocs/op PASS ok go.opentelemetry.io/otel/sdk/log 10.789s ``` Prior art: - #6271 - #6286 I also created for tracking purposes: - #6328
This introduces a new
go.opentelemetry.io/otel/sdk/log/xlogmodule.I created it because of two reasons:
go.opentelemetry.io/otel/sdk/logmodule does not expose these experimental feature in its exported API; it is only available in docs and example test. The user would have to explicitly opt-in by using thego.opentelemetry.io/otel/sdk/log/xlogmodule which will always havev0.x.yversions. Hopefully at some point of time the features are added togo.opentelemetry.io/otel/sdk/log, deprecated ingo.opentelemetry.io/otel/sdk/log/xlogand afterwards removed fromgo.opentelemetry.io/otel/sdk/log/xlog. I think that for at one release we would need to support both the deprecatedgo.opentelemetry.io/otel/sdk/xlog.FilterLoggerand newgo.opentelemetry.io/otel/sdk/log.FilterLoggerto facilitate the migration process. The module name is inspired byxerrors.xlogis undersdk/logso that it could have access to packages undersdk/log/internalif necessary.The SDK depends on
xlogusing. However, the SDK does not expose anything fromxlog(apart for documentation).xreceiverhas a similar design. More: #6271 (comment)As we can see calling
Enableddoes not cause any heap allocation and has speed similar to emitting a "smallest" log record: #6315